-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
TST: Added regression test case #31536
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
||
class TestNumericArraylikeArithmeticWithBool: | ||
@pytest.mark.skipif( | ||
compat.is_platform_32bit(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm what’s this for? Should still work on 32 bit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WillAyd Can you please look at the reasons b38f9e6 failed.
That's what I am understanding, from looking at it.
Maybe you have other conclusions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its just a bug somewhere; we typically don't default back to platform ints
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WillAyd Should I put a pytest.mark.skipif
decorator (skiping this test, if running on 32-bit platform) at least for now?
"rtruediv", | ||
"truediv", | ||
]: | ||
pytest.skip("Arithmetic operation is not supported") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably better to xfail than skip here; don’t think it matters when used imperatively but former would be more fitting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this something that should be supported but isnt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this something that should be supported but isnt?
I am not sure.
off-topic @MomIsBestFriend if you're up for suggestions on things needing attention, in core.indexing L1983 is never reached in the tests and i think it may be impossible, could use a fresh set of eyes to see if we can prove that hypothesis. Also L892-901 is not reached, but i dont have a strong intuition for if that is possible. |
@jbrockmendel Sure! I always like it when you give me tasks:) Thank you, means a lot 👍 |
re-started azure |
"rtruediv", | ||
"truediv", | ||
]: | ||
pytest.xfail("Arithmetic operation is not supported") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this not supported as in "not meaningful and so never will be supported" or as in "we havent gotten around to it yet so the behavior is wrong"? only the latter should be xfail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure, maybe @jorisvandenbossche knows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hard to see what the test is actually testing, but I would think something like this?
In [5]: pd.Series([True]) * 3
Out[5]:
0 3
dtype: int64
but that doesn't fail, though.
But I agree with @jbrockmendel: if there are cases that it raises an error purposefully, we should test that and not xfail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not able to test this without having if/else
statement in the code, which is not really a pattern we want in our tests (I think), Any thought on how to write this test case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The if/else might be fine, but in this if block, you can do a pytest.raises instead of xfail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to test the actual errors, will result in having a test case that looks somewhat like this:
@pytest.mark.parametrize(
"op",
[
operator.mul,
ops.rmul,
operator.floordiv,
ops.rfloordiv,
operator.truediv,
ops.rtruediv,
operator.pow,
ops.rpow,
operator.mod,
ops.rmod,
],
)
@pytest.mark.parametrize("num", [np.int64(1), 1, 1.0])
def test_array_like_bool_and_num_op_coerce_raises(self, op, num, box_with_array):
# https://github.com/pandas-dev/pandas/issues/18549
bool_box = [True]
if op.__name__ in {"foo", "bar", "baz"}:
msg = "|".join(
[
"can't mod complex numbers",
"can't take floor of complex number",
r"unsupported operand type\(s\) for %: 'float' and 'Index'",
"cannot perform __foo__ with this index type: Index",
]
)
with pytest.raises(TypeError, match=msg):
expected = [op(num, num)]
return
else:
expected = [op(num, num)]
bool_box = tm.box_expected(bool_box, box_with_array)
expected = tm.box_expected(expected, box_with_array)
if op.__name__ in {"foo", "bar", "baz"}:
msg = "|".join(["cannot perform __foo__ with this index type: Index"])
with pytest.raises(TypeError, match=msg):
tm.assert_equal(expected, op(bool_box, num))
else:
tm.assert_equal(expected, op(bool_box, num))
tm.assert_equal(expected, op(num, bool_box))
This looks sloppy, and not really readable, any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks fine to me in general (have some comments on some details, but that will be easier to add when this is in the diff)
|
||
|
||
class TestNumericArraylikeArithmeticWithBool: | ||
@pytest.mark.parametrize("num", [1, 1.0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
complex(1), np.int64(1), ... We may even have a fixture just called "one"
expected = tm.box_expected(expected, box_with_array) | ||
bool_box = tm.box_expected([True], box_with_array) | ||
tm.assert_equal(expected, op(bool_box, num)) | ||
tm.assert_equal(expected, op(num, bool_box)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick, the pattern we usually use is:
bool_box = [True]
expected = [op(num, num)]
bool_box = tm.box_expected(...
expected = tm.box_expected(...
result = op(bool_box, num)
tm.assert_equal(...
result = op(num, bool_box)
tm.assert_equal(...
Closing this, as this PR is giving me more headache than I expected. |
Resurrection of #28966
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff